🚀 Clear assertion greyed-out lines on restart and highlight issue locations #232
Conversation
svenjeschmitt-ops
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/mqt/debugger/dap/dap_server.py (1)
373-389: Handle empty source code edge case incode_pos_to_coordinates.When
source_codeis empty,scopes_dap_messagecalls this method withend_pos = -1, causing it to returnline = -1whenlines_start_at_oneisFalse. This produces invalid line numbers in DAP responses. Add an early check to handle empty source code explicitly or validate thatpos >= 0.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
python/mqt/debugger/dap/dap_server.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (13)
python/mqt/debugger/dap/dap_server.py (13)
118-119: LGTM! Clean state management additions.The new fields
pending_highlightsand_prevent_exitare properly initialized and serve the PR objective of managing highlight lifecycle and preventing premature exit when errors need to be displayed.
172-186: LGTM! Proper reset of exit prevention flag.Resetting
_prevent_exittoFalsewhen execution commands are issued ensures that the debugger can exit normally after the user takes action, aligning with the PR objective of preventing exit only while highlights are being displayed.
257-261: LGTM! Proper highlight emission and exit prevention.The logic correctly sends queued highlights, clears the pending list, and sets the exit prevention flag to ensure users can review the highlighted errors before the debugger exits.
271-275: LGTM! Exit gated by highlight state.The modified condition correctly prevents the debugger from exiting when
_prevent_exitisTrue, ensuring highlights remain visible until the user continues or steps.
355-361: LGTM! Proper error categorization and highlight emission.Using
"stderr"for assertion failure messages is appropriate, and the highlight collection logic ensures error causes are properly displayed to the user.
431-457: LGTM! Robust error cause collection with fallback.The method properly handles missing diagnostics and ensures at least the failing instruction is highlighted when no specific error causes are identified.
459-478: LGTM! Well-structured highlight entry construction.The method correctly handles position-to-coordinate conversion with proper clamping at Line 466 to prevent out-of-bounds coordinates. The snippet extraction at Line 468 relies on Python's graceful slice handling, which is acceptable since the snippet is for display purposes only.
480-487: LGTM! Clean error type mapping.Simple, focused method with proper default handling for unknown error types.
489-494: LGTM! Parse error queuing for deferred display.The method correctly parses and queues parse errors. Note that Line 494 replaces any existing pending highlights rather than appending; this is likely intentional for parse errors which typically occur individually during launch.
496-508: LGTM! Robust parse error location extraction.The regex pattern correctly matches typical compiler error format
<input>:line:column: detailwith appropriate fallback values when the pattern doesn't match.
546-569: LGTM! Comprehensive message flattening logic.The recursive flattening correctly handles nested message structures (strings, dicts, lists). For typical DAP message hierarchies, the recursion depth should be shallow and safe. If deeply nested messages become a concern, consider adding a depth limit parameter.
571-638: LGTM! Clean message sending refactor.The introduction of
send_message_simpleand the addition of thecategoryparameter tosend_message_hierarchyimproves code organization and allows proper categorization of messages (console/stdout/stderr).
640-656: LGTM! Clear execution state reporting.The method correctly reports execution state with appropriate priority (assertion failure > breakpoint > finished > running) and uses the new simple message pathway for clean output.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/dd/DDSimDebug.cpp (1)
591-627: Consolidate redundant state clearing into ddsimLoadCode.Lines 597–608 clear the same state containers that
preprocessAssertionCodeclears again at lines 1654–1665. SincepreprocessAssertionCodeis only called from this function (line 611) after the initial clearing, the duplicate clearing inpreprocessAssertionCodeis redundant and obscures where state initialization actually occurs.Consider removing the clearing loops from
preprocessAssertionCode(lines 1654–1665) and documenting that callers must ensure a clean state beforehand, or makepreprocessAssertionCodemore self-contained if it may be called independently in the future.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/backend/dd/DDSimDebug.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
🧬 Code graph analysis (1)
src/backend/dd/DDSimDebug.cpp (2)
test/utils/common_fixtures.hpp (6)
code(211-215)code(211-213)code(236-244)code(236-237)code(254-256)code(254-254)include/frontend/cli/CliFrontEnd.hpp (2)
code(46-46)code(68-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (1)
src/backend/dd/DDSimDebug.cpp (1)
1134-1151: LGTM: Readiness guard prevents stale state execution.The check on line 1136–1138 properly gates execution to ensure code is loaded and state is initialized. Combined with the
readyflag management (setfalsewhen loading code,trueafter successful processing), this ensures each execution starts with a clean slate and prevents undefined behavior from uninitialized state.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/backend/dd/DDSimDebug.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (1)
src/backend/dd/DDSimDebug.cpp (1)
593-608: LGTM: Proper state reset on code load.The explicit clearing of all instruction-related metadata and setting
ready = falseensures each execution starts with a clean state. This aligns with the PR's objective to reset greyed-out highlights between executions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
bindings/InterfaceBindings.cppsrc/common/parsing/CodePreprocessing.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-15T01:54:22.129Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/dd/register_matrix_dds.cpp:64-109
Timestamp: 2025-12-15T01:54:22.129Z
Learning: In the munich-quantum-toolkit/core repository, after migrating to nanobind, docstrings for Python bindings are now added directly in the C++ binding code (using R"pb(...)pb" syntax) and stub files (.pyi) are auto-generated using the `bindings/generate-stubs.sh` script. This replaces the previous pybind11 approach where docstrings were manually maintained in stub files.
Applied to files:
bindings/InterfaceBindings.cpp
🧬 Code graph analysis (1)
bindings/InterfaceBindings.cpp (1)
test/utils/common_fixtures.hpp (6)
code(211-215)code(211-213)code(236-244)code(236-237)code(254-256)code(254-254)
🪛 Cppcheck (2.19.0)
src/common/parsing/CodePreprocessing.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
bindings/InterfaceBindings.cpp
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (14)
bindings/InterfaceBindings.cpp (2)
26-26: LGTM! Includes are necessary for error capture.The
<iostream>header providesstd::cerrand<pybind11/iostream.h>providespy::scoped_ostream_redirect, both of which are used in the updatedload_codebinding.Also applies to: 29-29
177-193: Excellent improvement to error reporting!The new implementation captures stderr output during
loadCodeexecution via Python'sStringIO, providing users with actual parse error messages instead of generic error text. The use ofpy::scoped_ostream_redirectensures proper cleanup through RAII, and the fallback message prevents empty errors.src/common/parsing/CodePreprocessing.cpp (12)
23-23: LGTM! Necessary includes for new functionality.The includes support
std::isdigit(line 43) andstd::exception(lines 146, 501).Also applies to: 25-25
38-44: LGTM! Robust digit validation.The empty-string check and unsigned char cast properly handle edge cases for
std::isdigit.
46-49: LGTM! Clear structure for line/column positions.The 1-based initialization aligns with standard editor conventions.
67-91: LGTM! Well-designed target position fallback.The function gracefully falls back to the first non-space character when the target is not found, providing helpful error positioning.
93-99: LGTM! Standard error message formatting.The
<input>:line:column:format follows common compiler conventions for error reporting.
101-108: LGTM! Clear error message construction.
110-115: LGTM! Clear error message construction.
117-162: LGTM! Comprehensive target validation with robust error handling.The validation properly handles:
- Format checking (register name must be non-empty, proper bracket nesting)
- Numeric index validation with overflow protection
- Shadowed register handling
- Bounds checking
The try-catch around
std::stoul(lines 144-150) correctly guards against numeric overflow even afterisDigitsvalidation.
463-467: LGTM! Improved error positioning accuracy.Skipping leading whitespace ensures error highlights point to the actual instruction start rather than preceding spaces, improving the user experience.
493-504: LGTM! Robust register validation with detailed error reporting.The strengthened validation correctly handles:
- Missing sizes
- Non-numeric sizes
- Negative sizes (via
isDigitsrejecting '-')- Overflow (via exception handling)
The formatted error messages with line/column info significantly improve the debugging experience.
569-570: LGTM! Consolidated validation for assertion targets.Delegating to
validateTargetseliminates code duplication and provides consistent error formatting across all validation paths.
575-578: LGTM! Consolidated validation for regular instruction targets.The conditional check properly excludes variable declarations from target validation, as they have distinct syntax rules validated separately (lines 493-504).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (1)
58-84: Inconsistent message validation between normalization and validation.At line 79,
_normalize_highlightsetsmessageto""as a default for missing or whitespace-only messages. However,validate()(lines 48-50) later requires each message to be non-empty. This defers the error from normalization to validation.Consider failing immediately during normalization if a non-empty message is required, so callers receive earlier feedback.
🔎 Proposed fix
Validate the message during normalization:
normalized["instruction"] = int(normalized.get("instruction", -1)) normalized["reason"] = str(normalized.get("reason", "unknown")) normalized["code"] = str(normalized.get("code", "")) normalized["message"] = str(normalized.get("message", "")).strip() + if not normalized["message"]: + msg = "Each highlight entry must contain a descriptive 'message'." + raise ValueError(msg) normalized["range"] = { "start": start, "end": end, }Then simplify
validate()to only check that highlights is non-empty, since per-highlight validation is already done:def validate(self) -> None: """Validate the 'highlightError' DAP event message after creation.""" if not self.highlights: msg = "At least one highlight entry is required to show the issue location." raise ValueError(msg) - - for highlight in self.highlights: - if "message" not in highlight or not str(highlight["message"]).strip(): - msg = "Each highlight entry must contain a descriptive 'message'." - raise ValueError(msg)src/common/parsing/CodePreprocessing.cpp (1)
51-65: Bounds validation still missing.The previous review comment regarding the lack of bounds validation for
offsetremains applicable. Ifoffsetexceedscode.size(), the column calculation on line 63 will produce incorrect results.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.pysrc/common/parsing/CodePreprocessing.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/parsing/CodePreprocessing.cpp (3)
include/common/parsing/ParsingError.hpp (1)
ParsingError(32-32)src/common/parsing/ParsingError.cpp (1)
ParsingError(23-23)include/common/parsing/CodePreprocessing.hpp (1)
isVariableDeclaration(296-296)
🪛 Cppcheck (2.19.0)
src/common/parsing/CodePreprocessing.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
🔇 Additional comments (7)
src/common/parsing/CodePreprocessing.cpp (7)
38-44: LGTM!The
isDigitsimplementation correctly handles the empty string case and properly casts tounsigned charbefore callingstd::isdigitto avoid undefined behavior with negative char values.
67-91: LGTM!The function correctly calculates line/column positions for error reporting, with appropriate fallback to the first non-whitespace character when the target is not found.
93-115: LGTM!Clean utility functions for consistent error message formatting.
117-162: LGTM!The
validateTargetsfunction is well-implemented with:
- Proper bracket syntax validation
- Guarded numeric parsing with
isDigitscheck followed by try/catch for overflow protection- Correct handling of shadowed registers (function parameters)
- Proper bounds checking against defined register sizes
463-467: LGTM!The improved
trueStartcalculation correctly skips leading whitespace for more accurate error position reporting while properly maintaining bounds within the current instruction.
493-504: LGTM!The strengthened register declaration validation provides:
- Proper rejection of declarations missing size specifications
- Guarded numeric parsing with both
isDigitsand try/catch for overflow edge cases- Descriptive error messages with accurate position information
569-578: LGTM!Proper integration of
validateTargetsfor both assertions and regular instructions, with the correct guard to skip variable declarations which don't have qubit targets to validate.
python/mqt/debugger/dap/messages/highlight_error_dap_message.py
Outdated
Show resolved
Hide resolved
svenjeschmitt-ops
changed the title
svenjeschmitt-ops
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
36-45: Critical: Type signature mismatch -Nonenot handled.The constructor allows
source: Mapping[str, Any] | None, but_normalize_source(line 153) raisesTypeErrorwhensourceis not aMapping. PassingNonecauses a runtime exception.🔎 Recommended fix
Either handle
Noneexplicitly in_normalize_source:@staticmethod def _normalize_source(source: Mapping[str, Any] | None) -> dict[str, Any]: """Create a defensive copy of the provided DAP Source information.""" + if source is None: + msg = "Source information is required." + raise ValueError(msg) if not isinstance(source, Mapping): msg = "Source information must be provided as a mapping." raise TypeError(msg)Or remove
| Nonefrom the constructor ifsourceis always required:-def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any] | None) -> None: +def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any]) -> None:
47-61: Validation partially duplicates normalization checks.
validate()checks for non-empty highlights and messages, while_normalize_highlight(lines 86-98) validates range structure. There's overlap in that both methods touch the highlights list and message field, thoughvalidate()enforces non-empty messages post-normalization while_normalize_highlightonly normalizes. Consider consolidating to reduce duplication.python/mqt/debugger/dap/dap_server.py (1)
513-547: Minor: Redundant condition on Line 542.Line 542's ternary
end_column if end_column > 0 else columnis redundant. Sincecolumn >= 1(line 521) andend_column = max(column, len(line_text) + 1)(line 536),end_columnis always positive.Otherwise, the empty-line heuristic (lines 525-534) is a thoughtful UX improvement for locating errors on blank lines.
🔎 Optional simplification
return { "instruction": -1, "range": { "start": {"line": line, "column": column}, - "end": {"line": line, "column": end_column if end_column > 0 else column}, + "end": {"line": line, "column": end_column}, }, "reason": "parseError", "code": snippet, "message": detail, }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
python/mqt/debugger/dap/dap_server.pypython/mqt/debugger/dap/messages/highlight_error_dap_message.py
🧰 Additional context used
🧬 Code graph analysis (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (1)
python/mqt/debugger/dap/messages/dap_event.py (1)
DAPEvent(17-41)
python/mqt/debugger/dap/dap_server.py (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
HighlightError(22-179)encode(62-70)python/mqt/debugger/dap/messages/output_dap_event.py (2)
encode(54-69)OutputDAPEvent(18-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (18)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (4)
62-70: LGTM: Encoding follows DAP event pattern.The
encode()method correctly extends the base event structure and wraps the normalized data underbody, consistent with other DAP event implementations.
72-109: LGTM: Robust normalization with validation.
_normalize_highlightprovides thorough validation of range structure and ordering, with sensible defaults for optional fields. The defensive checks and error messages are clear.
111-137: LGTM: Position normalization is defensive.The type check and KeyError handling ensure required fields are present and convertible to integers, with clear error messages for debugging.
139-162: Source normalization is correct but see constructor issue.The validation and normalization logic is sound for non-
Nonesources. However, this method's rejection ofNonecreates the type mismatch issue flagged in the constructor review (lines 36-45).python/mqt/debugger/dap/dap_server.py (14)
14-14: LGTM: Import supports parse error handling.The
remodule import is used for parsing error message locations in_parse_error_location(line 502).
118-119: LGTM: State variables properly initialized.
pending_highlightsand_prevent_exitare correctly typed and initialized to support the highlight queuing and exit prevention logic.
172-186: LGTM: Exit prevention reset on execution commands.Resetting
_prevent_exitfor execution-resuming commands (Next, Step*, Continue, Restart, Launch) correctly allows exit events after clean execution while preventing them after parse/assertion errors.
257-261: LGTM: Highlight emission and exit prevention.The logic correctly emits queued highlights, clears the queue, and sets
_prevent_exitto prevent premature exit events after parse errors.
271-275: LGTM: Exit condition respects prevention flag.The condition correctly prevents
ExitedDAPEventwhen_prevent_exitisTrue, allowing the session to remain active after parse or assertion errors for user review.
357-361: LGTM: Assertion failure highlights emitted.Collecting and emitting highlight entries for assertion failures with
_prevent_exitcorrectly prevents premature session termination while displaying error context.
363-389: LGTM: Position calculation handles edge cases.The logic correctly handles empty source, positions exceeding the source length, and normal cases. The
for-elseclause appropriately handles overflow by using the last line's bounds.
431-457: LGTM: Highlight collection with defensive fallback.The method defensively handles diagnostic errors and provides a sensible fallback to highlight the failing instruction when no specific error causes are identified.
459-481: LGTM: Highlight entry construction is defensive.The method safely handles runtime errors from position lookups and appropriately calculates end positions to exclude trailing newlines for cleaner highlighting.
483-490: LGTM: Error cause mapping is clear.The mapping from
ErrorCauseTypeto string identifiers is straightforward, with a sensible default for unknown types.
492-497: LGTM: Parse error queuing is appropriate.Extracting location from the error message and replacing
pending_highlightsis correct for parse errors, which occur once per launch/restart.
499-511: LGTM: Error location parsing with fallback.The regex pattern correctly parses compiler error format, with a sensible fallback to line 1, column 1 when the format doesn't match.
549-641: LGTM: Message sending refactored for modularity.The refactoring into
_flatten_message_partsandsend_message_simpleimproves code organization. The recursive flattening correctly handles nested message structures (strings, dicts, lists), and the delegation fromsend_message_hierarchytosend_message_simplereduces duplication.
643-659: LGTM: State reporting is comprehensive.The method checks all relevant execution states (assertion failed, breakpoint, finished, running) and sends clear status messages to the client using the appropriate messaging infrastructure.
DRovara
left a comment
DRovara
left a comment
There was a problem hiding this comment.
Thanks a lot for the work and sorry for the late review. This ended up becoming quite a big PR, so I hope I have covered everything. Either way, at some point I would probably also like to manually try everything out myself, but I guess that can wait until after we merge this.
I have a couple of requested changes. Some of them should be quite simple, but there are also some bigger ones that might need larger restructuring. I hope that works out fine for you.
python/mqt/debugger/dap/messages/highlight_error_dap_message.py
Outdated
Show resolved
Hide resolved
| def _format_highlight_reason(cause_type: mqt.debugger.ErrorCauseType | None) -> str: | ||
| """Return a short identifier for the highlight reason.""" | ||
| if cause_type == mqt.debugger.ErrorCauseType.MissingInteraction: | ||
| return "missingInteraction" |
There was a problem hiding this comment.
Can we maybe use a "cleaner" datatype - i.e., an enum - for these indicators?
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @python/mqt/debugger/dap/messages/highlight_error_dap_message.py:
- Around line 134-137: The returned position dict lacks validation: before the
return in the method that currently returns {"line": line, "column": column} (in
highlight_error_dap_message.py) add checks that both line and column are
integers and >= 0, and raise a clear exception (e.g., ValueError or TypeError)
with a message mentioning the invalid field and value; this ensures malformed
position data is caught early and surfaces a helpful error message.
- Around line 100-104: The int() conversion for normalized["instruction"] can
raise ValueError for non-numeric input; wrap the conversion in a try/except
(catch ValueError and TypeError) and fall back to -1 (or another safe default)
when parsing fails, and likewise guard any other risky casts (e.g., to str) if
needed; update the block that sets normalized["instruction"],
normalized["reason"], normalized["code"], and normalized["message"] so parsing
errors don’t propagate and normalized always receives safe default values.
- Around line 128-133: The try/except that parses position["line"] and
position["column"] only catches KeyError but int() can raise ValueError; update
the exception handling in the position parsing block (the code that sets line =
int(position["line"]) and column = int(position["column"]) in
highlight_error_dap_message.py) to catch both KeyError and ValueError, and
adjust the raised ValueError message to clearly indicate whether keys are
missing or values are not valid integers (e.g., differentiate "missing
'line'/'column' keys" vs "invalid integer value for 'line'/'column'").
- Around line 164-179: The _end_before_start method is overly defensive: after
positions are validated/normalized by _normalize_position you should access
numeric fields directly instead of using .get(...) with defaults and int()
conversions; update _end_before_start to use start["line"], start["column"],
end["line"], end["column"] (and treat them as ints already) so missing or
non-numeric values raise immediately, making the assumption that callers used
_normalize_position explicit.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py
🧰 Additional context used
🧬 Code graph analysis (1)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
python/mqt/debugger/dap/messages/dap_event.py (1)
DAPEvent(17-41)python/mqt/debugger/dap/messages/launch_dap_message.py (1)
validate(44-55)
🔇 Additional comments (4)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (4)
22-45: LGTM! Well-structured class definition and constructor.The class correctly extends
DAPEvent, defines appropriate type-annotated attributes, and follows a sound initialization pattern by normalizing inputs before validation.
47-61: LGTM! Appropriate validation logic.The validation correctly ensures required data is present and meaningful. Structural validation is appropriately handled earlier during normalization in the constructor.
62-70: LGTM! Clean encoding implementation.The method correctly extends the base event encoding with the highlight-specific payload.
139-162: LGTM! Solid source normalization.The method properly validates required fields and safely normalizes string values with clear error messages.
| normalized = dict(entry) | ||
| normalized["instruction"] = int(normalized.get("instruction", -1)) | ||
| normalized["reason"] = str(normalized.get("reason", "unknown")) | ||
| normalized["code"] = str(normalized.get("code", "")) | ||
| normalized["message"] = str(normalized.get("message", "")).strip() |
There was a problem hiding this comment.
Add error handling for type conversions.
The int() conversion on line 101 will raise ValueError if the "instruction" field exists but contains a non-numeric value (e.g., "abc"). This would cause initialization to fail unexpectedly.
🛡️ Proposed fix with proper error handling
normalized = dict(entry)
- normalized["instruction"] = int(normalized.get("instruction", -1))
+ try:
+ normalized["instruction"] = int(normalized.get("instruction", -1))
+ except (ValueError, TypeError) as exc:
+ msg = f"Highlight 'instruction' must be convertible to int, got: {normalized.get('instruction')}"
+ raise ValueError(msg) from exc
normalized["reason"] = str(normalized.get("reason", "unknown"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| normalized = dict(entry) | |
| normalized["instruction"] = int(normalized.get("instruction", -1)) | |
| normalized["reason"] = str(normalized.get("reason", "unknown")) | |
| normalized["code"] = str(normalized.get("code", "")) | |
| normalized["message"] = str(normalized.get("message", "")).strip() | |
| normalized = dict(entry) | |
| try: | |
| normalized["instruction"] = int(normalized.get("instruction", -1)) | |
| except (ValueError, TypeError) as exc: | |
| msg = f"Highlight 'instruction' must be convertible to int, got: {normalized.get('instruction')}" | |
| raise ValueError(msg) from exc | |
| normalized["reason"] = str(normalized.get("reason", "unknown")) | |
| normalized["code"] = str(normalized.get("code", "")) | |
| normalized["message"] = str(normalized.get("message", "")).strip() |
🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/messages/highlight_error_dap_message.py around lines
100 - 104, The int() conversion for normalized["instruction"] can raise
ValueError for non-numeric input; wrap the conversion in a try/except (catch
ValueError and TypeError) and fall back to -1 (or another safe default) when
parsing fails, and likewise guard any other risky casts (e.g., to str) if
needed; update the block that sets normalized["instruction"],
normalized["reason"], normalized["code"], and normalized["message"] so parsing
errors don’t propagate and normalized always receives safe default values.
| try: | ||
| line = int(position["line"]) | ||
| column = int(position["column"]) | ||
| except KeyError as exc: | ||
| msg = "Highlight positions require 'line' and 'column'." | ||
| raise ValueError(msg) from exc |
There was a problem hiding this comment.
Catch ValueError in addition to KeyError.
The try-except block only catches KeyError but the int() conversions on lines 129-130 can also raise ValueError if the "line" or "column" values exist but aren't convertible to integers (e.g., "abc", 3.14).
🔧 Proposed fix
try:
line = int(position["line"])
column = int(position["column"])
- except KeyError as exc:
+ except (KeyError, ValueError, TypeError) as exc:
msg = "Highlight positions require 'line' and 'column'."
raise ValueError(msg) from excYou may also want to enhance the error message to distinguish between missing keys and invalid values:
try:
line = int(position["line"])
column = int(position["column"])
- except KeyError as exc:
- msg = "Highlight positions require 'line' and 'column'."
+ except KeyError as exc:
+ msg = "Highlight positions require 'line' and 'column' keys."
+ raise ValueError(msg) from exc
+ except (ValueError, TypeError) as exc:
+ msg = f"Highlight position 'line' and 'column' must be numeric, got line={position.get('line')}, column={position.get('column')}"
raise ValueError(msg) from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| line = int(position["line"]) | |
| column = int(position["column"]) | |
| except KeyError as exc: | |
| msg = "Highlight positions require 'line' and 'column'." | |
| raise ValueError(msg) from exc | |
| try: | |
| line = int(position["line"]) | |
| column = int(position["column"]) | |
| except (KeyError, ValueError, TypeError) as exc: | |
| msg = "Highlight positions require 'line' and 'column'." | |
| raise ValueError(msg) from exc |
🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/messages/highlight_error_dap_message.py around lines
128 - 133, The try/except that parses position["line"] and position["column"]
only catches KeyError but int() can raise ValueError; update the exception
handling in the position parsing block (the code that sets line =
int(position["line"]) and column = int(position["column"]) in
highlight_error_dap_message.py) to catch both KeyError and ValueError, and
adjust the raised ValueError message to clearly indicate whether keys are
missing or values are not valid integers (e.g., differentiate "missing
'line'/'column' keys" vs "invalid integer value for 'line'/'column'").
| return { | ||
| "line": line, | ||
| "column": column, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding bounds validation for position values.
While not strictly required, validating that line and column are non-negative integers would catch malformed position data earlier and provide clearer error messages.
📏 Example validation addition
return {
"line": line,
"column": column,
}
+ if line < 0 or column < 0:
+ msg = f"Position line and column must be non-negative, got line={line}, column={column}"
+ raise ValueError(msg)
+ return {
+ "line": line,
+ "column": column,
+ }Note: Place the validation before the return statement in the actual implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| "line": line, | |
| "column": column, | |
| } | |
| if line < 0 or column < 0: | |
| msg = f"Position line and column must be non-negative, got line={line}, column={column}" | |
| raise ValueError(msg) | |
| return { | |
| "line": line, | |
| "column": column, | |
| } |
🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/messages/highlight_error_dap_message.py around lines
134 - 137, The returned position dict lacks validation: before the return in the
method that currently returns {"line": line, "column": column} (in
highlight_error_dap_message.py) add checks that both line and column are
integers and >= 0, and raise a clear exception (e.g., ValueError or TypeError)
with a message mentioning the invalid field and value; this ensures malformed
position data is caught early and surfaces a helpful error message.
| @staticmethod | ||
| def _end_before_start(start: Mapping[str, Any], end: Mapping[str, Any]) -> bool: | ||
| """Return True if 'end' describes a position before 'start'. | ||
| Args: | ||
| start (Mapping[str, Any]): The start position mapping. | ||
| end (Mapping[str, Any]): The end position mapping. | ||
| Returns: | ||
| bool: True when the end position is before the start position. | ||
| """ | ||
| start_line = int(start.get("line", 0)) | ||
| start_column = int(start.get("column", 0)) | ||
| end_line = int(end.get("line", 0)) | ||
| end_column = int(end.get("column", 0)) | ||
| return (end_line, end_column) < (start_line, start_column) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying position access after normalization.
This method is called after _normalize_position has already validated and normalized the positions, so using .get() with defaults is unnecessarily defensive and could mask issues. Additionally, the int() conversions could theoretically raise ValueError if the values aren't numeric.
♻️ Proposed refactor assuming positions are already normalized
Since positions are normalized before this method is called, you can directly access the keys:
@staticmethod
def _end_before_start(start: Mapping[str, Any], end: Mapping[str, Any]) -> bool:
"""Return True if 'end' describes a position before 'start'.
Args:
start (Mapping[str, Any]): The start position mapping.
end (Mapping[str, Any]): The end position mapping.
Returns:
bool: True when the end position is before the start position.
"""
- start_line = int(start.get("line", 0))
- start_column = int(start.get("column", 0))
- end_line = int(end.get("line", 0))
- end_column = int(end.get("column", 0))
+ start_line = start["line"]
+ start_column = start["column"]
+ end_line = end["line"]
+ end_column = end["column"]
return (end_line, end_column) < (start_line, start_column)This makes the assumption explicit that positions are already validated, and any missing keys would raise a clear KeyError.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def _end_before_start(start: Mapping[str, Any], end: Mapping[str, Any]) -> bool: | |
| """Return True if 'end' describes a position before 'start'. | |
| Args: | |
| start (Mapping[str, Any]): The start position mapping. | |
| end (Mapping[str, Any]): The end position mapping. | |
| Returns: | |
| bool: True when the end position is before the start position. | |
| """ | |
| start_line = int(start.get("line", 0)) | |
| start_column = int(start.get("column", 0)) | |
| end_line = int(end.get("line", 0)) | |
| end_column = int(end.get("column", 0)) | |
| return (end_line, end_column) < (start_line, start_column) | |
| @staticmethod | |
| def _end_before_start(start: Mapping[str, Any], end: Mapping[str, Any]) -> bool: | |
| """Return True if 'end' describes a position before 'start'. | |
| Args: | |
| start (Mapping[str, Any]): The start position mapping. | |
| end (Mapping[str, Any]): The end position mapping. | |
| Returns: | |
| bool: True when the end position is before the start position. | |
| """ | |
| start_line = start["line"] | |
| start_column = start["column"] | |
| end_line = end["line"] | |
| end_column = end["column"] | |
| return (end_line, end_column) < (start_line, start_column) |
🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/messages/highlight_error_dap_message.py around lines
164 - 179, The _end_before_start method is overly defensive: after positions are
validated/normalized by _normalize_position you should access numeric fields
directly instead of using .get(...) with defaults and int() conversions; update
_end_before_start to use start["line"], start["column"], end["line"],
end["column"] (and treat them as ints already) so missing or non-numeric values
raise immediately, making the assumption that callers used _normalize_position
explicit.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @python/mqt/debugger/dap/dap_server.py:
- Line 547: The ternary in the dict construction is redundant because earlier
code ensures column = max(1, column) and end_column = max(column, len(line_text)
+ 1), so end_column is always > 0; update the object at the "end" key to simply
use end_column (replace "end_column if end_column > 0 else column" with
"end_column") so the code is simpler and clearer; the involved symbols are
column, end_column and the dict entry "end" in the function around dap_server.py
lines where column and end_column are computed.
- Line 541: The end_column calculation currently uses "end_column = max(column,
len(line_text) + 1)" which can push the range past the line when column >
len(line_text); change the logic around the end_column assignment (where
end_column, column, and line_text are used) to clamp the end to the actual line
length (e.g., set end_column = len(line_text) + 1 or otherwise min(column,
len(line_text) + 1) so the highlight never extends beyond the line); update any
related range creation to rely on this clamped end_column.
- Around line 453-460: The fallback call to _build_highlight_entry can raise
AttributeError because _build_highlight_entry reads self.source_code; add a
guard at the start of _build_highlight_entry to return None (or skip building)
when getattr(self, "source_code", "") is falsy, so the code path that appends
the returned entry to highlights is safe; alternatively, move the fallback block
that calls _build_highlight_entry inside the existing getattr(self,
"source_code", "") check so the method is only called when source_code exists.
In @src/backend/dd/DDSimDebug.cpp:
- Around line 626-636: The catch block can be simplified by removing the
redundant empty-string check after assigning e.what() to
ddsim->lastErrorMessage; instead just set ddsim->lastErrorMessage = e.what() and
return ERROR (keep the existing catch(...) fallback unchanged), so update the
catch (const std::exception& e) handler around ddsim->lastErrorMessage to
eliminate the unnecessary conditional while preserving the fallback in the
non-standard catch-all path.
- Line 1153: Remove the unused local variable by deleting the line that declares
"auto* ddsim = toDDSimulationState(self);" in the function (the variable is
unused and all operations use the "self" pointer); ensure no other references to
"ddsim" remain and rebuild to confirm no unused-variable warnings persist.
- Around line 603-619: The container-clearing in preprocessAssertionCode
duplicates the clearing already performed in ddsimLoadCode (which is the only
caller); remove the redundant calls to clear instructionTypes,
assertionInstructions, successorInstructions, variable-related containers, and
any other state cleared earlier from preprocessAssertionCode, or consolidate
initialization so preprocessAssertionCode only performs assertion-specific
setup; reference the functions preprocessAssertionCode and ddsimLoadCode when
making the change and ensure no required initialization is lost.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
python/mqt/debugger/dap/dap_server.pysrc/backend/dd/DDSimDebug.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
src/backend/dd/DDSimDebug.cpp
🧬 Code graph analysis (1)
src/backend/dd/DDSimDebug.cpp (2)
include/frontend/cli/CliFrontEnd.hpp (2)
code(46-46)code(68-68)include/backend/dd/DDSimDebug.hpp (1)
preprocessAssertionCode(678-678)
🔇 Additional comments (17)
src/backend/dd/DDSimDebug.cpp (4)
524-524: LGTM!The interface assignment correctly exposes the new error message retrieval function.
584-584: LGTM!Clearing
lastErrorMessageduring initialization ensures a clean error state at startup, which aligns with the PR's objective to reset highlights on restart.
78-84: No issues found. The functionddsimGetLastErrorMessagesafely returns a pointer to internal string data viac_str(). All callers in the codebase use the pointer immediately or convert it tostd::stringwithout caching, which prevents any lifetime issues.
1424-1428: Confirm or document the intended breakpoint semantics for out-of-range positions.The early-return logic at lines 1424–1428 sets a breakpoint at instruction
iwhendesiredPosition < start. This means breakpoints requested before any instruction starts are placed at that first instruction. The function also returns ERROR only whendesiredPositionexceeds all instruction boundaries.The behavior appears intentional—breaking at the next reachable instruction when the exact position has no code. However, no test cases confirm this semantic, and there is no API documentation clarifying whether this behavior is by design or if alternative strategies (e.g., breaking at a previous instruction or searching within sub-gates first) were considered. Add test coverage or document the expected behavior to make the design intent clear.
python/mqt/debugger/dap/dap_server.py (13)
14-14: LGTM: Import additions support new functionality.The
remodule is needed for parse error location extraction, andAnysupports the new type hints for highlight data structures.Also applies to: 17-17
118-119: LGTM: State fields for coordinated highlight emission.These fields enable the post-command highlighting flow described in the PR objectives.
172-186: LGTM: Coordinated highlight emission prevents premature exit.The reset-then-set flow for
_prevent_exitensures that highlights are displayed before the extension exits, addressing the bug described in the PR objectives.Also applies to: 257-261
271-275: LGTM: Exit guard ensures highlights remain visible.The added condition prevents the extension from exiting while error highlights are displayed.
355-361: LGTM: Structured highlight emission for assertion failures.The new flow using
collect_highlight_entriesand the HighlightError event provides structured diagnostic data. Using "stderr" for assertion failure messages is appropriate.
373-389: LGTM: Bug fix for coordinate calculation edge case.The added
elseclause correctly handles the case whereposexceeds the length of all lines, preventing incorrect line/column calculations. The defensive checkif linesis reasonable, thoughsplit("\n")always returns a non-empty list.
464-486: LGTM: Careful boundary handling for highlight ranges.The method correctly handles edge cases where
end_posmight be at or beyond the source code length, and appropriately adjusts for newline characters.
488-495: LGTM: Clean mapping for error cause types.The static method provides a straightforward mapping from error cause types to highlight reason identifiers.
497-502: LGTM: Parse error queueing for deferred emission.The method correctly queues parse error highlights for emission after command processing.
554-577: LGTM: Recursive flattening handles nested message structures.The method correctly handles the various message structure types (strings, dicts with title/body/end, lists, and other types) and flattens them into a list of strings.
579-646: LGTM: Clean refactoring with category support.The refactored message sending now properly supports output categories (console/stdout/stderr), and the separation between
send_message_hierarchyandsend_message_simpleimproves code clarity.
648-664: LGTM: State reporting provides execution status.The method appropriately reports the current execution state through the simple message path, supporting the debugging workflow.
504-516: The error message format is stable—it's hardcoded insrc/common/parsing/CodePreprocessing.cpp(lines 97–98) as"<input>:" + line + ":" + column + ": " + detail, which exactly matches the regex pattern. No changes to this format are needed or flagged as a concern.
| if not highlights: | ||
| entry = self._build_highlight_entry( | ||
| failing_instruction, | ||
| "assertionFailed", | ||
| "Assertion failed at this instruction.", | ||
| ) | ||
| if entry is not None: | ||
| highlights.append(entry) |
There was a problem hiding this comment.
Critical: Potential crash if source_code is not set.
The fallback highlight creation on line 454 is outside the if getattr(self, "source_code", ""): guard, but _build_highlight_entry accesses self.source_code without checking if it exists (line 471, 476). If source_code is not set, this will raise an AttributeError.
🔧 Proposed fix: Guard the fallback highlight creation
for cause in error_causes:
message = self.format_error_cause(cause)
reason = self._format_highlight_reason(cause.type)
entry = self._build_highlight_entry(cause.instruction, reason, message)
if entry is not None:
highlights.append(entry)
- if not highlights:
- entry = self._build_highlight_entry(
- failing_instruction,
- "assertionFailed",
- "Assertion failed at this instruction.",
- )
- if entry is not None:
- highlights.append(entry)
+ if not highlights:
+ entry = self._build_highlight_entry(
+ failing_instruction,
+ "assertionFailed",
+ "Assertion failed at this instruction.",
+ )
+ if entry is not None:
+ highlights.append(entry)
return highlightsAlternatively, add a guard at the beginning of _build_highlight_entry:
def _build_highlight_entry(self, instruction: int, reason: str, message: str) -> dict[str, Any] | None:
"""Create a highlight entry for a specific instruction."""
+ if not getattr(self, "source_code", ""):
+ return None
try:
start_pos, end_pos = self.simulation_state.get_instruction_position(instruction)
except RuntimeError:
return None🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/dap_server.py around lines 453 - 460, The fallback
call to _build_highlight_entry can raise AttributeError because
_build_highlight_entry reads self.source_code; add a guard at the start of
_build_highlight_entry to return None (or skip building) when getattr(self,
"source_code", "") is falsy, so the code path that appends the returned entry to
highlights is safe; alternatively, move the fallback block that calls
_build_highlight_entry inside the existing getattr(self, "source_code", "")
check so the method is only called when source_code exists.
| if column <= 1 and line_index > 0 and not line_text.strip(): | ||
| prev_index = line_index - 1 | ||
| while prev_index >= 0 and not lines[prev_index].strip(): | ||
| prev_index -= 1 | ||
| if prev_index >= 0: | ||
| line_index = prev_index | ||
| line = line_index + 1 | ||
| line_text = lines[line_index] | ||
| stripped = line_text.lstrip() | ||
| column = max(1, len(line_text) - len(stripped) + 1) if stripped else 1 |
There was a problem hiding this comment.
Minor: Heuristic adjustment to previous line may not always be appropriate.
When a parse error points to an empty line at column 1, the code adjusts the highlight to the previous non-empty line. While this improves UX for common cases (e.g., missing semicolon reported on next line), it may be incorrect if the error genuinely relates to a missing statement on the empty line.
Consider whether this heuristic should be configurable or documented as a known behavior.
| stripped = line_text.lstrip() | ||
| column = max(1, len(line_text) - len(stripped) + 1) if stripped else 1 | ||
|
|
||
| end_column = max(column, len(line_text) + 1) |
There was a problem hiding this comment.
Major: Incorrect end_column calculation can create invalid ranges.
When column exceeds len(line_text), the calculation max(column, len(line_text) + 1) produces an end_column that extends even further beyond the line. This creates a highlight range that extends past the line's actual content.
📐 Proposed fix: Clamp end_column to line length
- end_column = max(column, len(line_text) + 1)
+ end_column = max(column + 1, min(len(line_text) + 1, column + 1)) if column <= len(line_text) else column + 1Or more simply, always highlight to the end of the line:
- end_column = max(column, len(line_text) + 1)
+ end_column = len(line_text) + 1This ensures the highlight extends to the end of the line regardless of where the column points.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| end_column = max(column, len(line_text) + 1) | |
| end_column = max(column + 1, min(len(line_text) + 1, column + 1)) if column <= len(line_text) else column + 1 |
| end_column = max(column, len(line_text) + 1) | |
| end_column = len(line_text) + 1 |
🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/dap_server.py at line 541, The end_column
calculation currently uses "end_column = max(column, len(line_text) + 1)" which
can push the range past the line when column > len(line_text); change the logic
around the end_column assignment (where end_column, column, and line_text are
used) to clamp the end to the actual line length (e.g., set end_column =
len(line_text) + 1 or otherwise min(column, len(line_text) + 1) so the highlight
never extends beyond the line); update any related range creation to rely on
this clamped end_column.
| "instruction": -1, | ||
| "range": { | ||
| "start": {"line": line, "column": column}, | ||
| "end": {"line": line, "column": end_column if end_column > 0 else column}, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Remove unnecessary ternary condition.
Given that column = max(1, column) (line 526) and end_column = max(column, len(line_text) + 1) (line 541), end_column is always ≥ 1, making the condition end_column > 0 always true.
🧹 Simplification
- "end": {"line": line, "column": end_column if end_column > 0 else column},
+ "end": {"line": line, "column": end_column},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "end": {"line": line, "column": end_column if end_column > 0 else column}, | |
| "end": {"line": line, "column": end_column}, |
🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/dap_server.py at line 547, The ternary in the dict
construction is redundant because earlier code ensures column = max(1, column)
and end_column = max(column, len(line_text) + 1), so end_column is always > 0;
update the object at the "end" key to simply use end_column (replace "end_column
if end_column > 0 else column" with "end_column") so the code is simpler and
clearer; the involved symbols are column, end_column and the dict entry "end" in
the function around dap_server.py lines where column and end_column are
computed.
| ddsim->ready = false; | ||
| ddsim->code = code; | ||
| ddsim->variables.clear(); | ||
| ddsim->variableNames.clear(); | ||
| ddsim->instructionTypes.clear(); | ||
| ddsim->instructionStarts.clear(); | ||
| ddsim->instructionEnds.clear(); | ||
| ddsim->functionDefinitions.clear(); | ||
| ddsim->assertionInstructions.clear(); | ||
| ddsim->successorInstructions.clear(); | ||
| ddsim->classicalRegisters.clear(); | ||
| ddsim->qubitRegisters.clear(); | ||
| ddsim->dataDependencies.clear(); | ||
| ddsim->functionCallers.clear(); | ||
| ddsim->targetQubits.clear(); | ||
| ddsim->instructionObjects.clear(); | ||
| ddsim->lastErrorMessage.clear(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, let's find the ddsimLoadCode function and understand its structure
rg -n "ddsimLoadCode" src/backend/dd/DDSimDebug.cpp -A 30 | head -80Repository: munich-quantum-toolkit/debugger
Length of output: 3335
🏁 Script executed:
# Also search for preprocessAssertionCode to understand the call relationship
rg -n "preprocessAssertionCode" src/backend/dd/DDSimDebug.cpp -B 2 -A 5Repository: munich-quantum-toolkit/debugger
Length of output: 786
🏁 Script executed:
# Let's check the line ranges mentioned in the review (603-619 and 1674-1685)
sed -n '603,619p' src/backend/dd/DDSimDebug.cppRepository: munich-quantum-toolkit/debugger
Length of output: 645
🏁 Script executed:
# Check the other clearing location at 1674-1685
sed -n '1674,1685p' src/backend/dd/DDSimDebug.cppRepository: munich-quantum-toolkit/debugger
Length of output: 500
🏁 Script executed:
# Let's also verify the full context around ddsimLoadCode to see if preprocessAssertionCode is called
sed -n '600,650p' src/backend/dd/DDSimDebug.cppRepository: munich-quantum-toolkit/debugger
Length of output: 1672
🏁 Script executed:
# Find all calls to preprocessAssertionCode to see if it's called from elsewhere
rg -n "preprocessAssertionCode" src/backend/dd/DDSimDebug.cppRepository: munich-quantum-toolkit/debugger
Length of output: 201
🏁 Script executed:
# Also check header files for declarations or other calls
fd -e h -e hpp src/backend/dd/ | xargs rg "preprocessAssertionCode" 2>/dev/null || trueRepository: munich-quantum-toolkit/debugger
Length of output: 750
🏁 Script executed:
# Broader search across the entire codebase
rg -n "preprocessAssertionCode" --type cpp --type hRepository: munich-quantum-toolkit/debugger
Length of output: 380
LGTM! State clearing ensures clean execution state.
The comprehensive clearing of state tracking containers (including instructionTypes, assertionInstructions, etc.) ensures that greyed-out highlights and other state from previous executions are properly reset, which directly addresses the PR's objective. Setting ready = false prevents operations during loading.
However, the clearing at lines 1674-1685 in preprocessAssertionCode is redundant—since preprocessAssertionCode is only called from ddsimLoadCode (line 622), all containers cleared in preprocessAssertionCode are already cleared beforehand. Consider removing the duplicate clearing in preprocessAssertionCode for code clarity.
🤖 Prompt for AI Agents
In @src/backend/dd/DDSimDebug.cpp around lines 603 - 619, The container-clearing
in preprocessAssertionCode duplicates the clearing already performed in
ddsimLoadCode (which is the only caller); remove the redundant calls to clear
instructionTypes, assertionInstructions, successorInstructions, variable-related
containers, and any other state cleared earlier from preprocessAssertionCode, or
consolidate initialization so preprocessAssertionCode only performs
assertion-specific setup; reference the functions preprocessAssertionCode and
ddsimLoadCode when making the change and ensure no required initialization is
lost.
| } catch (const std::exception& e) { | ||
| std::cerr << e.what() << "\n"; | ||
| ddsim->lastErrorMessage = e.what(); | ||
| if (ddsim->lastErrorMessage.empty()) { | ||
| ddsim->lastErrorMessage = | ||
| "An error occurred while executing the operation"; | ||
| } | ||
| return ERROR; | ||
| } catch (...) { | ||
| ddsim->lastErrorMessage = "An error occurred while executing the operation"; | ||
| return ERROR; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good error handling enhancement, minor optimization suggestion.
The new error handling properly captures exception messages in lastErrorMessage and provides a fallback message. This allows the frontend to display meaningful error information via the new getLastErrorMessage API.
Minor optimization: The empty check on line 628 is defensive but may be unnecessary since std::exception::what() typically returns a non-empty string. However, keeping it is safer for robustness.
🤖 Prompt for AI Agents
In @src/backend/dd/DDSimDebug.cpp around lines 626 - 636, The catch block can be
simplified by removing the redundant empty-string check after assigning e.what()
to ddsim->lastErrorMessage; instead just set ddsim->lastErrorMessage = e.what()
and return ERROR (keep the existing catch(...) fallback unchanged), so update
the catch (const std::exception& e) handler around ddsim->lastErrorMessage to
eliminate the unnecessary conditional while preserving the fallback in the
non-standard catch-all path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @python/mqt/debugger/dap/dap_server.py:
- Around line 518-552: The function _build_parse_error_highlight has untested
edge cases and a clarity bug: change the condition currently written as `column
<= 1` to `column == 1` (since column is already clamped to >=1) in the branch
that walks backwards across blank lines, and add unit tests that exercise parse
errors on an empty-first-line file, on a whitespace-only line, and on multiple
consecutive empty lines to assert the returned highlight uses the previous
non-empty line, computes the column correctly (indent start) and sets a
reasonable end_column/snippet/message; target tests at the same behavior covered
by _build_parse_error_highlight so they fail if the branch is incorrect.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
python/mqt/debugger/dap/dap_server.py
🧰 Additional context used
🧬 Code graph analysis (1)
python/mqt/debugger/dap/dap_server.py (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
HighlightError(22-179)encode(62-70)python/mqt/debugger/dap/messages/output_dap_event.py (2)
encode(54-69)OutputDAPEvent(18-69)
🔇 Additional comments (17)
python/mqt/debugger/dap/dap_server.py (17)
14-14: LGTM: Import additions are appropriate.The
remodule is used for parsing error messages (line 507), andAnyis used for type hints throughout the new methods.Also applies to: 17-17
118-119: LGTM: State fields support the highlight error feature.
pending_highlightsqueues highlight data for deferred emission, and_prevent_exitprevents premature termination when errors are displayed to the user.
172-186: LGTM: Restores normal exit behavior after user interaction.Resetting
_prevent_exitafter stepping or restart commands allows the debugger to exit normally once the user has acknowledged and acted on highlighted errors.
257-261: LGTM: Deferred highlight emission works correctly.Pending highlights (queued by parse errors) are emitted after command processing, and
_prevent_exitis set to ensure the user can view them before the session terminates.
271-275: LGTM: Exit condition properly respects the prevent-exit flag.Adding
not self._prevent_exitensures the debugger remains active when error highlights are displayed, preventing premature session termination.
355-355: LGTM: Appropriate category for error output.Using
"stderr"for assertion failure messages correctly categorizes them as errors rather than normal console output.
357-361: LGTM: Assertion failures now emit structured highlights.The new code collects highlight entries for error causes and emits them as
HighlightErrorevents, enabling rich UI feedback for debugging assertion failures.
431-462: LGTM: Robust highlight collection with fallback.The method correctly collects highlights for error causes and provides a sensible fallback (highlighting the failing instruction) when diagnostics are unavailable or produce no causes. Exception handling at line 443 prevents crashes.
464-486: LGTM: Highlight entry construction is correct.The method carefully distinguishes between:
end_position_exclusive(line 471-474): For UI highlight range, excludes trailing newlines- Snippet extraction (line 476): Includes the character at
end_pos, then strips whitespaceThis ensures precise UI highlighting while capturing the full instruction text for display.
488-495: LGTM: Simple and safe error cause mapping.The method maps error cause types to string identifiers with a safe fallback to
"unknown".
497-502: Verify that only one parse error can occur per launch.Line 502 replaces the entire
pending_highlightslist with a single entry. If multiple parse errors could occur during a single launch/restart, only the last one would be displayed.If multiple parse errors are possible, consider appending instead:
if entry is not None: self.pending_highlights.append(entry)Otherwise, document that only the first/last parse error is shown.
504-516: LGTM: Robust parse error location extraction.The regex correctly parses compiler error format, and the fallback to
(1, 1, error_message)ensures the method never fails even with unexpected error formats.
554-577: LGTM: Robust recursive message flattening.The method correctly handles nested message structures with appropriate filtering of empty strings and None values. The recursive approach cleanly processes dictionaries, lists, and primitive types.
579-606: LGTM: Message hierarchy refactoring preserves functionality.The refactored method correctly extracts message components, flattens nested structures, and delegates to the new
send_message_simplemethod. The addedcategoryparameter is backward compatible with its default value.
607-646: LGTM: New message sending method is clean and focused.The method consolidates message formatting and transmission into a single, straightforward function with clear parameter separation. The early return (line 636) handles empty messages gracefully.
118-119: The grey-out clearing mechanism is correctly implemented, though not through explicit clear events. Instead, grey-outs are implicitly reset by recalculation:
- On restart:
RestartDAPMessage.handle()callsreset_simulation()to clear state, then loads new code- Grey-out reset occurs automatically:
regular_checks()recalculatesgray_out_areasfrom scratch based on the current instruction's dependencies, and sends a freshGrayOutDAPEventwith the updated areas- This happens after every command including Launch and Restart, ensuring old grey-outs from a previous execution are replaced with correct grey-outs for the new state
The implementation correctly clears assertion-related highlights via
pending_highlights(emitted asHighlightErrorevents) which is separate from the grey-out mechanism. No changes needed.Likely an incorrect or invalid review comment.
373-389: The coordinate conversion changes are correct and well-tested.The modifications properly handle edge cases:
- Line 373: Initializes to line 1 for non-empty source
- Line 376: The
<=condition correctly places positions at newlines on the current line (verified bytest_code_pos_to_coordinates_handles_line_end())- Lines 381-384: Fallback for positions extending beyond the file
All improvements are substantiated and the critical boundary condition change is covered by tests.
| def _build_parse_error_highlight(self, line: int, column: int, detail: str) -> dict[str, Any] | None: | ||
| """Create a highlight entry for a parse error.""" | ||
| if not getattr(self, "source_code", ""): | ||
| return None | ||
| lines = self.source_code.split("\n") | ||
| if not lines: | ||
| return None | ||
| line = max(1, min(line, len(lines))) | ||
| column = max(1, column) | ||
| line_index = line - 1 | ||
| line_text = lines[line_index] | ||
|
|
||
| if column <= 1 and line_index > 0 and not line_text.strip(): | ||
| prev_index = line_index - 1 | ||
| while prev_index >= 0 and not lines[prev_index].strip(): | ||
| prev_index -= 1 | ||
| if prev_index >= 0: | ||
| line_index = prev_index | ||
| line = line_index + 1 | ||
| line_text = lines[line_index] | ||
| stripped = line_text.lstrip() | ||
| column = max(1, len(line_text) - len(stripped) + 1) if stripped else 1 | ||
|
|
||
| end_column = max(column, len(line_text) + 1) | ||
| snippet = line_text.strip() or line_text | ||
| return { | ||
| "instruction": -1, | ||
| "range": { | ||
| "start": {"line": line, "column": column}, | ||
| "end": {"line": line, "column": end_column if end_column > 0 else column}, | ||
| }, | ||
| "reason": "parseError", | ||
| "code": snippet, | ||
| "message": detail, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find test files related to dap_server
find . -type f -name "*test*.py" | grep -i dap
# Search for existing tests for parse error highlighting
rg -n 'test.*parse.*error|test.*highlight' --type=py | head -30
# Check if _build_parse_error_highlight is tested
rg -n '_build_parse_error_highlight' --type=pyRepository: munich-quantum-toolkit/debugger
Length of output: 476
🏁 Script executed:
#!/bin/bash
# Read the test file to see what's being tested
cat -n ./test/python/test_dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 2008
🏁 Script executed:
#!/bin/bash
# Check total lines in test file
wc -l ./test/python/test_dap_server.py
# Get any remaining content after line 44
tail -n +45 ./test/python/test_dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Read the _build_parse_error_highlight method again for detailed analysis
sed -n '518,552p' python/mqt/debugger/dap/dap_server.py | cat -nRepository: munich-quantum-toolkit/debugger
Length of output: 1737
Add test coverage for edge cases in _build_parse_error_highlight.
The method handles several edge cases (empty lines, invalid positions) to provide sensible highlights, but lacks test coverage for them. The existing single test (test_build_highlight_entry_does_not_span_next_instruction) only covers normal code without edge cases.
Additionally, line 530's condition column <= 1 is imprecise: since column is guaranteed >= 1 from line 526, this effectively checks column == 1 and should be written as such for clarity.
Add tests for:
- Parse errors on empty lines at the start of a file
- Parse errors on whitespace-only lines
- Parse errors on multiple consecutive empty lines
🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/dap_server.py around lines 518 - 552, The function
_build_parse_error_highlight has untested edge cases and a clarity bug: change
the condition currently written as `column <= 1` to `column == 1` (since column
is already clamped to >=1) in the branch that walks backwards across blank
lines, and add unit tests that exercise parse errors on an empty-first-line
file, on a whitespace-only line, and on multiple consecutive empty lines to
assert the returned highlight uses the previous non-empty line, computes the
column correctly (indent start) and sets a reasonable
end_column/snippet/message; target tests at the same behavior covered by
_build_parse_error_highlight so they fail if the branch is incorrect.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/common/parsing/CodePreprocessing.cpp:
- Around line 163-208: The catch in validateTargets around the std::stoul call
is too broad (catching std::exception); change it to catch the specific
exceptions thrown by std::stoul—catch const std::invalid_argument& and const
std::out_of_range& (either as two catches or a single multi-catch) and in each
handler rethrow the ParsingError as currently done; keep the surrounding logic
and error message intact and do not swallow other exception types.
- Around line 62-76: The loop that counts newlines in LineColumn
lineColumnForOffset should be replaced with std::count for clarity and potential
performance: compute auto newlineCount = std::count(code.begin(), code.begin() +
lineStart, '\n') and set location.line = 1 + newlineCount instead of iterating
up to lineStart; keep the existing calculation of lineStart and location.column
unchanged and ensure to include <algorithm> if not already included.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/common/parsing/CodePreprocessing.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/parsing/CodePreprocessing.cpp (3)
include/common/parsing/ParsingError.hpp (1)
ParsingError(32-32)src/common/parsing/ParsingError.cpp (1)
ParsingError(23-23)include/common/parsing/CodePreprocessing.hpp (1)
isVariableDeclaration(296-296)
🪛 Cppcheck (2.19.0)
src/common/parsing/CodePreprocessing.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
🔇 Additional comments (8)
src/common/parsing/CodePreprocessing.cpp (8)
23-25: LGTM: Includes support new validation logic.The added includes are appropriate for the new digit checking and exception handling functionality.
43-49: LGTM: Robust digit validation.The function correctly validates non-empty digit-only strings with proper casting for
std::isdigit.
85-109: LGTM: Appropriate fallback for error location.The function correctly locates the target token or falls back to the first non-space character, which is reasonable for error reporting.
119-152: LGTM: Clear error formatting utilities.The error formatting follows standard compiler conventions and provides helpful context for users.
509-513: LGTM: Improved error location accuracy.The adjusted
trueStartcalculation correctly identifies the actual start of the instruction by finding the first non-space character, improving error reporting precision.
539-550: LGTM: Robust register validation with clear error messages.The validation correctly checks for empty names and non-digit sizes before parsing. The exception handling is consistent with
validateTargets(lines 189-196), though both could benefit from more specific exception types as noted in that function's review.
615-616: LGTM: Comprehensive assertion validation.The call to
validateTargetsis appropriately placed after unfolding and internal validation, with clear context for error messages.
621-624: LGTM: Appropriate target validation for instructions.The validation correctly skips variable declarations (which have dedicated handling at lines 534-552) and validates targets for all other instructions.
| LineColumn lineColumnForOffset(const std::string& code, size_t offset) { | ||
| LineColumn location; | ||
| const auto lineStartPos = code.rfind('\n', offset); | ||
| const size_t lineStart = (lineStartPos == std::string::npos) | ||
| ? 0 | ||
| : static_cast<size_t>(lineStartPos + 1); | ||
| location.line = 1; | ||
| for (size_t i = 0; i < lineStart; i++) { | ||
| if (code[i] == '\n') { | ||
| location.line++; | ||
| } | ||
| } | ||
| location.column = offset - lineStart + 1; | ||
| return location; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider std::count for line counting.
The newline counting loop could be replaced with std::count(code.begin(), code.begin() + lineStart, '\n') for improved performance and readability, though this is not a critical path.
♻️ Optional refactor
LineColumn lineColumnForOffset(const std::string& code, size_t offset) {
LineColumn location;
const auto lineStartPos = code.rfind('\n', offset);
const size_t lineStart = (lineStartPos == std::string::npos)
? 0
: static_cast<size_t>(lineStartPos + 1);
- location.line = 1;
- for (size_t i = 0; i < lineStart; i++) {
- if (code[i] == '\n') {
- location.line++;
- }
- }
+ location.line = 1 + std::count(code.begin(), code.begin() + lineStart, '\n');
location.column = offset - lineStart + 1;
return location;
}🤖 Prompt for AI Agents
In @src/common/parsing/CodePreprocessing.cpp around lines 62 - 76, The loop that
counts newlines in LineColumn lineColumnForOffset should be replaced with
std::count for clarity and potential performance: compute auto newlineCount =
std::count(code.begin(), code.begin() + lineStart, '\n') and set location.line =
1 + newlineCount instead of iterating up to lineStart; keep the existing
calculation of lineStart and location.column unchanged and ensure to include
<algorithm> if not already included.
| void validateTargets(const std::string& code, size_t instructionStart, | ||
| const std::vector<std::string>& targets, | ||
| const std::map<std::string, size_t>& definedRegisters, | ||
| const std::vector<std::string>& shadowedRegisters, | ||
| const std::string& context) { | ||
| for (const auto& target : targets) { | ||
| if (target.empty()) { | ||
| continue; | ||
| } | ||
| const auto open = target.find('['); | ||
| if (open == std::string::npos) { | ||
| continue; | ||
| } | ||
| const auto close = target.find(']', open + 1); | ||
| if (open == 0 || close == std::string::npos || close != target.size() - 1) { | ||
| throw ParsingError(formatParseError(code, instructionStart, | ||
| invalidTargetDetail(target, context), | ||
| target)); | ||
| } | ||
| const auto registerName = target.substr(0, open); | ||
| const auto indexText = target.substr(open + 1, close - open - 1); | ||
| if (!isDigits(indexText)) { | ||
| throw ParsingError(formatParseError(code, instructionStart, | ||
| invalidTargetDetail(target, context), | ||
| target)); | ||
| } | ||
| size_t registerIndex = 0; | ||
| try { | ||
| registerIndex = std::stoul(indexText); | ||
| } catch (const std::exception&) { | ||
| throw ParsingError(formatParseError(code, instructionStart, | ||
| invalidTargetDetail(target, context), | ||
| target)); | ||
| } | ||
| if (std::ranges::find(shadowedRegisters, registerName) != | ||
| shadowedRegisters.end()) { | ||
| continue; | ||
| } | ||
| const auto found = definedRegisters.find(registerName); | ||
| if (found == definedRegisters.end() || found->second <= registerIndex) { | ||
| throw ParsingError(formatParseError(code, instructionStart, | ||
| invalidTargetDetail(target, context), | ||
| target)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more specific exception catching.
The catch block at line 192 catches std::exception, which is quite broad. std::stoul specifically throws std::invalid_argument and std::out_of_range. While catching the base class is safe, being more specific improves code clarity.
♻️ Proposed refactor
size_t registerIndex = 0;
try {
registerIndex = std::stoul(indexText);
- } catch (const std::exception&) {
+ } catch (const std::invalid_argument&) {
+ throw ParsingError(formatParseError(code, instructionStart,
+ invalidTargetDetail(target, context),
+ target));
+ } catch (const std::out_of_range&) {
throw ParsingError(formatParseError(code, instructionStart,
invalidTargetDetail(target, context),
target));
}🤖 Prompt for AI Agents
In @src/common/parsing/CodePreprocessing.cpp around lines 163 - 208, The catch
in validateTargets around the std::stoul call is too broad (catching
std::exception); change it to catch the specific exceptions thrown by
std::stoul—catch const std::invalid_argument& and const std::out_of_range&
(either as two catches or a single multi-catch) and in each handler rethrow the
ParsingError as currently done; keep the surrounding logic and error message
intact and do not swallow other exception types.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
python/mqt/debugger/dap/dap_server.py (2)
579-646:send_message_hierarchy/send_message_simplerefactor looks good; adjust docstrings + avoidstr(None)title.
Currentlytitle = str(message.get("title", ""))can produce"None".Proposed tweak
@@ - title = str(message.get("title", "")) + raw_title = message.get("title") + title = raw_title if isinstance(raw_title, str) else "" self.send_message_simple(title, body, end, line, column, connection, category)
412-463: Handle unhandledErrorCauseTypevalues with a default message instead of returning empty string.The
format_error_cause()method uses a nested ternary operator that returns""for anyErrorCauseTypenot explicitly handled (currently onlyMissingInteractionandControlAlwaysZeroare covered). If mqt-core adds newErrorCauseTypevariants in the future, those would be silently formatted as empty messages, potentially violating DAP protocol message constraints.Proposed fix
@@ for cause in error_causes: - message = self.format_error_cause(cause) + message = self.format_error_cause(cause).strip() + if not message: + # Keep HighlightError payload valid even when we don't have a dedicated formatter yet. + message = "Potential error cause at this instruction." reason = self._format_highlight_reason(cause.type) entry = self._build_highlight_entry(cause.instruction, reason, message) if entry is not None: highlights.append(entry)Also applies to: 464-487
python/mqt/debugger/dap/messages/restart_dap_message.py (1)
57-76: Fix NameError (parsed_successfullyis undefined) and handleload_codeRuntimeError as non-fatal (align with Launch).
RestartDAPMessage.handle()on line 73 references undefined variableparsed_successfully, causing aNameErrorat runtime. Additionally, unlikeLaunchDAPMessage, the restart handler doesn't catchRuntimeErrorfromload_code()(line 70) and will crash on parse failures. The response also incorrectly returns"command": "launch"instead of"restart"(line 80).Wrap
load_code()in try-except, defineparsed_successfully, return proper error response on parse failure, and fix the response command field to"restart".Proposed fix
@@ def handle(self, server: DAPServer) -> dict[str, Any]: @@ server.simulation_state.reset_simulation() program_path = Path(self.program) code = program_path.read_text(encoding=locale.getpreferredencoding(False)) server.source_code = code - server.simulation_state.load_code(code) + server.source_file = {"name": program_path.name, "path": str(program_path)} + parsed_successfully = True + try: + parsed_successfully = bool(server.simulation_state.load_code(code)) + except RuntimeError as exc: + parsed_successfully = False + server.queue_parse_error(str(exc)) + return { + "type": "response", + "request_seq": self.sequence_number, + "success": False, + "command": "restart", + "message": "An error occurred while parsing the code.", + } if not self.stop_on_entry: server.simulation_state.run_simulation() if not parsed_successfully: with contextlib.suppress(RuntimeError): server.simulation_state.reset_simulation() + return { + "type": "response", + "request_seq": self.sequence_number, + "success": True, + "command": "restart", + }python/mqt/debugger/dap/messages/launch_dap_message.py (1)
66-83: Fix undefined variableparsed_successfullyon line 81 and ensureserver.source_fileis set.Line 81 references
parsed_successfullywhich is never defined, causing aNameErrorat runtime. Additionally,server.source_fileis not assigned in thehandle()method, breaking downstream calls toHighlightError(...)inDAPServer(which requiressourcewith"name"and"path"keys per line 157-159 ofhighlight_error_dap_message.py).Set
server.source_fileto{"name": program_path.name, "path": str(program_path)}after line 68, and initializeparsed_successfully = Truebefore the try block (or remove the unused check on line 81-83 if it serves no purpose given that exceptions return early on line 72).
🤖 Fix all issues with AI agents
In @python/mqt/debugger/dap/dap_server.py:
- Around line 497-552: The parse error detail can be empty causing
HighlightError.validate() to fail; update _parse_error_location(error_message)
to return a non-empty fallback (e.g., "parse error" or "unknown parse error")
when detail is empty or only whitespace, and ensure
queue_parse_error/_build_parse_error_highlight will propagate that non-empty
string (alternatively also guard in _build_parse_error_highlight by replacing an
empty detail with the same fallback) so the "message" field in the highlight
dict is never empty.
In @python/mqt/debugger/dap/messages/highlight_error_dap_message.py:
- Around line 111-137: The _normalize_position signature says it accepts
Mapping[str, Any] | None but the implementation doesn't handle None; either
remove | None from the type hint on _normalize_position to disallow None, or
explicitly handle None at the start of the function (e.g., if position is None:
raise TypeError("Highlight positions must be mappings with 'line' and
'column'.") ) so the runtime behavior matches the signature; update the function
annotation or add the explicit None check inside _normalize_position
accordingly.
In @src/backend/dd/DDSimDebug.cpp:
- Line 1153: In ddsimRunAll, the local pointer 'ddsim' assigned via
toDDSimulationState(self) is never used; either remove the unused declaration
entirely or, if you plan to use it later, add a clear comment above it
explaining it's intentionally unused or explicitly mark it as used (e.g.,
(void)ddsim) to avoid warnings; update the code around the 'ddsim' declaration
accordingly.
In @test/python/test_dap_server.py:
- Around line 28-44: The fake diagnostics object uses
potential_error_causes=list (the type) which is confusing; change
fake_diagnostics to set potential_error_causes to an actual empty list (e.g.,
potential_error_causes=[]) so the
test_build_highlight_entry_does_not_span_next_instruction more clearly supplies
a diagnostics object with no causes when server.simulation_state.get_diagnostics
is used by DAPServer.collect_highlight_entries.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
bindings/InterfaceBindings.cppinclude/backend/dd/DDSimDebug.hppinclude/backend/debug.hpython/mqt/debugger/dap/dap_server.pypython/mqt/debugger/dap/messages/__init__.pypython/mqt/debugger/dap/messages/highlight_error_dap_message.pypython/mqt/debugger/dap/messages/launch_dap_message.pypython/mqt/debugger/dap/messages/restart_dap_message.pysrc/backend/dd/DDSimDebug.cppsrc/common/parsing/CodePreprocessing.cppsrc/frontend/cli/CliFrontEnd.cpptest/python/test_dap_server.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
src/common/parsing/CodePreprocessing.cppsrc/backend/dd/DDSimDebug.cpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
src/backend/dd/DDSimDebug.cpp
🧬 Code graph analysis (8)
bindings/InterfaceBindings.cpp (2)
include/frontend/cli/CliFrontEnd.hpp (2)
code(46-46)code(68-68)test/utils/common_fixtures.hpp (6)
code(211-215)code(211-213)code(236-244)code(236-237)code(254-256)code(254-254)
python/mqt/debugger/dap/messages/__init__.py (1)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (1)
HighlightError(22-179)
python/mqt/debugger/dap/messages/restart_dap_message.py (1)
python/mqt/debugger/pydebugger.pyi (1)
reset_simulation(193-198)
python/mqt/debugger/dap/messages/launch_dap_message.py (1)
python/mqt/debugger/pydebugger.pyi (1)
reset_simulation(193-198)
src/common/parsing/CodePreprocessing.cpp (3)
include/common/parsing/ParsingError.hpp (1)
ParsingError(32-32)src/common/parsing/ParsingError.cpp (1)
ParsingError(23-23)include/common/parsing/CodePreprocessing.hpp (1)
isVariableDeclaration(296-296)
test/python/test_dap_server.py (2)
python/mqt/debugger/dap/dap_server.py (2)
code_pos_to_coordinates(363-389)collect_highlight_entries(431-462)python/mqt/debugger/pydebugger.pyi (3)
potential_error_causes(527-535)get_instruction_position(274-284)get_diagnostics(419-424)
src/backend/dd/DDSimDebug.cpp (1)
include/backend/dd/DDSimDebug.hpp (1)
preprocessAssertionCode(678-678)
python/mqt/debugger/dap/dap_server.py (3)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
HighlightError(22-179)encode(62-70)python/mqt/debugger/dap/messages/output_dap_event.py (2)
encode(54-69)OutputDAPEvent(18-69)python/mqt/debugger/pydebugger.pyi (4)
ErrorCause(436-445)get_diagnostics(419-424)get_instruction_position(274-284)ErrorCauseType(25-33)
🪛 Cppcheck (2.19.0)
src/common/parsing/CodePreprocessing.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
🪛 Ruff (0.14.10)
python/mqt/debugger/dap/messages/restart_dap_message.py
73-73: Undefined name parsed_successfully
(F821)
python/mqt/debugger/dap/messages/launch_dap_message.py
81-81: Undefined name parsed_successfully
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (24)
src/common/parsing/CodePreprocessing.cpp (11)
23-25: LGTM!The new includes are appropriate for the added functionality:
<cctype>forstd::isdigitand<exception>for the exception base class used in catch handlers.
43-49: LGTM!Good defensive implementation. The cast to
unsigned charbefore callingstd::isdigitis the correct approach to avoid undefined behavior with negative char values on platforms wherecharis signed.
62-76: LGTM!The line/column calculation logic is correct. The use of
rfindto find the start of the current line and the subsequent iteration to count newlines handles edge cases properly.
85-109: LGTM!The function correctly locates targets within a line and provides a sensible fallback to the first non-whitespace column when the target isn't found. This improves error message precision.
119-125: LGTM!The error format follows the standard compiler diagnostic style (
<input>:line:column: message), which enables IDE integration for clickable error locations.
133-152: LGTM!The helper functions provide consistent error message formatting for target and register validation errors.
163-208: LGTM!The validation function is comprehensive and correctly handles:
- Bracket syntax validation
- Numeric index validation with overflow protection via
isDigits+std::stoulwith try/catch- Shadowed register bypass for function parameters
- Bounds checking against defined register sizes
The belt-and-suspenders approach of checking
isDigitsbeforestd::stoulis appropriate sincestoulcan still throw on overflow for very large numbers.
509-513: LGTM!Good improvement to skip leading whitespace when computing
trueStart, resulting in more accurate error locations that point to actual code rather than indentation.
539-551: LGTM!Robust parsing with proper validation. The
isDigitscheck followed bystd::stoulwith exception handling protects against both non-numeric input and integer overflow, providing clear error messages in both cases.
615-616: LGTM!The validation order is correct: first unfold register references to individual qubits, then perform semantic validation via
a->validate(), and finally validate target references against defined registers.
621-624: LGTM!Correctly skips validation for variable declarations (which define registers rather than reference them) while validating targets for all other instruction types.
src/frontend/cli/CliFrontEnd.cpp (1)
72-80: LGTM! Improved error reporting for code loading failures.The implementation correctly checks for the presence of the
getLastErrorMessagefunction pointer before calling it, and provides a helpful fallback message when no detailed error is available. This significantly improves the debugging experience when code loading fails.include/backend/debug.h (1)
57-66: LGTM! Well-documented error retrieval API.The new
getLastErrorMessageinterface method is clearly documented with appropriate lifetime semantics. The placement afterloadCodeis logical, and the ownership model (implementation-owned pointer) is clearly stated.bindings/InterfaceBindings.cpp (1)
177-187: LGTM! Enhanced Python binding error messages.The error handling correctly captures the result, queries for a detailed error message when available, and provides an appropriate fallback. This brings the Python bindings in line with the improved error reporting available in other interfaces.
include/backend/dd/DDSimDebug.hpp (1)
122-125: LGTM! Storage for error messages added.The addition of the
lastErrorMessagemember provides the necessary storage to implement the error retrieval interface. Usingstd::stringis appropriate for managing the lifetime and memory of error messages.src/backend/dd/DDSimDebug.cpp (5)
78-84: LGTM! Correct implementation of error message retrieval.The function correctly returns
nullptrwhen no error message is available and otherwise returns the C-string from the ownedstd::string. The lifetime semantics are safe since the string is owned by theDDSimulationStateinstance.
524-524: LGTM! Interface wiring is correct.The assignment of
ddsimGetLastErrorMessageto the interface function pointer follows the established pattern for other interface methods.
584-584: LGTM! Error state cleared during initialization.Clearing
lastErrorMessageduring initialization ensures a clean state for each session.
603-635: LGTM! Comprehensive error handling with proper state management.The changes correctly:
- Set
ready = falseearly to prevent use of partially-loaded state- Clear all state containers including
lastErrorMessagefor a clean slate- Capture exception messages from both specific and generic exceptions
- Provide appropriate fallback messages when exception details are unavailable
- Return
ERRORafter recording the error messageThe error handling is robust and will significantly improve the debugging experience.
1424-1428: This breakpoint snapping logic is intentional and correct, but verify if this code was newly added in the current PR.The logic at lines 1424-1428 implements standard debugger behavior: when a breakpoint is requested at a position before an instruction starts, it snaps to that instruction. This is consistent with the documented behavior stating "MQT Debugger will then determine the instruction that corresponds to this location in the code" and is confirmed by the DAP implementation which returns the actual instruction position to the client.
However, if this is newly added code, confirm it's related to the PR's objectives. If pre-existing, this review comment can be resolved.
python/mqt/debugger/dap/messages/__init__.py (1)
24-62: Export ofHighlightErrorlooks consistent with existing package API patterns.test/python/test_dap_server.py (1)
18-25: Good regression coverage for newline boundary (pos at\n) incode_pos_to_coordinates.python/mqt/debugger/dap/messages/highlight_error_dap_message.py (1)
22-70: Solid, defensive event payload normalization + stablebodyshape (highlights/source).python/mqt/debugger/dap/dap_server.py (1)
271-275: Verify the intended DAP lifecycle when highlights are shown: The_prevent_exitflag suppresses theExitedevent when set toTrue(after sending highlights) and is only reset upon specific client commands (execution/restart). If the client closes the connection without issuing another command after highlights are shown, theExitedevent will never be sent, which may violate DAP protocol expectations. Confirm whether this is intentional for UX purposes or if the flag should be reset differently (e.g., after highlights are acknowledged by the client, or on disconnect).
| self.pending_highlights: list[dict[str, Any]] = [] | ||
| self._prevent_exit = False | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and locate the relevant code
cd python/mqt/debugger/dap
ls -laRepository: munich-quantum-toolkit/debugger
Length of output: 434
🏁 Script executed:
# Check lines 118-120 in dap_server.py (initialization section)
head -130 dap_server.py | tail -20Repository: munich-quantum-toolkit/debugger
Length of output: 147
🏁 Script executed:
# Check lines 257-261 in dap_server.py (highlight emission section)
sed -n '250,270p' dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 131
🏁 Script executed:
# Search for source_file initialization/assignments
rg -n 'source_file\s*=' dap_server.py -A 2 -B 2Repository: munich-quantum-toolkit/debugger
Length of output: 128
🏁 Script executed:
# Check HighlightError class definition and validate method
rg -n 'class HighlightError\|def validate' messages.py -A 5Repository: munich-quantum-toolkit/debugger
Length of output: 126
🏁 Script executed:
# Check DAPEvent class and encode method
rg -n 'class DAPEvent\|def encode' messages.py -A 3Repository: munich-quantum-toolkit/debugger
Length of output: 126
🏁 Script executed:
# Check lines 118-120 in dap_server.py (initialization section)
sed -n '110,130p' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 902
🏁 Script executed:
# Check lines 257-261 in dap_server.py (highlight emission section)
sed -n '250,270p' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1141
🏁 Script executed:
# Search for source_file initialization/assignments
rg -n 'source_file\s*=' python/mqt/debugger/dap/dap_server.py -A 2 -B 2Repository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Check HighlightError class definition in messages
rg -n 'class HighlightError' python/mqt/debugger/dap/messages -A 20Repository: munich-quantum-toolkit/debugger
Length of output: 2336
🏁 Script executed:
# Check DAPEvent class and encode method
rg -n 'class DAPEvent' python/mqt/debugger/dap/messages -A 10Repository: munich-quantum-toolkit/debugger
Length of output: 865
🏁 Script executed:
# Look for validate method in HighlightError or its parent
rg -n 'def validate' python/mqt/debugger/dap/messages -B 2 -A 10Repository: munich-quantum-toolkit/debugger
Length of output: 39056
🏁 Script executed:
# Get the complete HighlightError.validate() method
sed -n '47,75p' python/mqt/debugger/dap/messages/highlight_error_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1206
🏁 Script executed:
# Search for where source_file is assigned in dap_server.py
rg -n 'self\.source_file' python/mqt/debugger/dap/dap_server.py -B 2 -A 2Repository: munich-quantum-toolkit/debugger
Length of output: 1217
🏁 Script executed:
# Check if source_file is set in launch or initialize handlers
rg -n 'source_file' python/mqt/debugger/dap/messages -B 1 -A 1 | head -50Repository: munich-quantum-toolkit/debugger
Length of output: 1324
🏁 Script executed:
# Search for where source_file is assigned (initialization)
rg -n 'source_file\s*=' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Check the full context around line 258-259 and see if source_file is checked before use
sed -n '240,270p' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1752
🏁 Script executed:
# Check in messages/set_breakpoints_dap_message.py where source_file is accessed
sed -n '45,75p' python/mqt/debugger/dap/messages/set_breakpoints_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1477
🏁 Script executed:
# Search for initialization in launch or other startup handlers
rg -n 'self.source_file\s*=' python/mqt/debugger/dap/messages/ -B 2 -A 2Repository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Search more broadly for where source_file might be set in the entire dap module
rg -n 'source_file' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 469
🏁 Script executed:
# Check the LaunchDAPMessage or InitializeDAPMessage handlers to see if they set source_file
rg -n 'def handle' python/mqt/debugger/dap/messages/launch_dap_message.py -A 30 | head -50Repository: munich-quantum-toolkit/debugger
Length of output: 1354
🏁 Script executed:
# Search for where source_file gets populated
rg -n 'source_file.*:' python/mqt/debugger/dap/dap_server.py -B 3 -A 3Repository: munich-quantum-toolkit/debugger
Length of output: 277
🏁 Script executed:
# Look for where source_file is set - check the full __init__ and other methods
sed -n '100,200p' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 4151
🏁 Script executed:
# Check the initialize handler which likely sets source_file
rg -n 'def handle' python/mqt/debugger/dap/messages/initialize_dap_message.py -A 40 | head -60Repository: munich-quantum-toolkit/debugger
Length of output: 859
🏁 Script executed:
# Search for initialization of source_file attribute
rg -n 'server\.source_file\s*=' python/mqt/debugger/dap/messages -B 2 -A 2Repository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Let's check if source_file is ever populated anywhere - search entire dap module
rg -n 'source_file\s*=' python/mqt/debugger/dapRepository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Look at the complete launch_dap_message handle method
sed -n '57,95p' python/mqt/debugger/dap/messages/launch_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1304
🏁 Script executed:
# Check if maybe source_file is set in LaunchDAPMessage's handle or elsewhere
cat python/mqt/debugger/dap/messages/launch_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 2889
🏁 Script executed:
# Search across the entire codebase for any assignment to source_file
rg -rn 'source_file.*=' python/mqt/debugger/Repository: munich-quantum-toolkit/debugger
Length of output: 209
🏁 Script executed:
# Check the complete HighlightError.__init__ to see how it normalizes source
sed -n '36,50p' python/mqt/debugger/dap/messages/highlight_error_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 741
🏁 Script executed:
# Let's check the _normalize_highlight and related methods in HighlightError
sed -n '60,100p' python/mqt/debugger/dap/messages/highlight_error_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1692
🏁 Script executed:
# Check the _normalize_source method
sed -n '100,140p' python/mqt/debugger/dap/messages/highlight_error_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1625
🏁 Script executed:
# Check what happens when source_file is accessed - let's trace initialization flow
grep -n 'def __init__' python/mqt/debugger/dap/dap_server.py -A 20Repository: munich-quantum-toolkit/debugger
Length of output: 1038
🏁 Script executed:
# Search if source_file is perhaps initialized later in a superclass or elsewhere
rg -n 'class DAPServer' python/mqt/debugger/dap/dap_server.py -A 50 | head -80Repository: munich-quantum-toolkit/debugger
Length of output: 1987
🏁 Script executed:
# Check the complete _normalize_source method
sed -n '140,180p' python/mqt/debugger/dap/messages/highlight_error_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1724
🏁 Script executed:
# Let's trace where source_file might be set - check all files in messages folder
find python/mqt/debugger/dap/messages -name '*.py' -exec grep -l 'source_file' {} \;Repository: munich-quantum-toolkit/debugger
Length of output: 253
🏁 Script executed:
# Search to see if source_file is set in InitializeDAPMessage or anywhere before highlight emission
rg -n 'source_file' python/mqt/debugger/dap/messages/initialize_dap_message.py -B 2 -A 2Repository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Check if there's an exception handler around the highlight emission code
sed -n '245,275p' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1618
🏁 Script executed:
# Look for all usages of self.source_file to see the pattern
rg -n 'self\.source_file' python/mqt/debugger/dap/dap_server.py -B 1 -A 1Repository: munich-quantum-toolkit/debugger
Length of output: 777
🏁 Script executed:
# Check restart_dap_message to see if source_file is set there
cat python/mqt/debugger/dap/messages/restart_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 2673
🏁 Script executed:
# Let's check the entire flow - does any handler set server.source_file?
rg -rn 'server\.source' python/mqt/debugger/dap/messages/ | grep -v '.pyc'Repository: munich-quantum-toolkit/debugger
Length of output: 1054
🏁 Script executed:
# Check if maybe source_file gets populated in __init__ or somewhere we haven't seen
rg -n '\["name"\].*\["path"\]' python/mqt/debugger/dap -B 3 -A 1Repository: munich-quantum-toolkit/debugger
Length of output: 660
🏁 Script executed:
# Let's verify - are there any tests that show how source_file should be initialized?
find . -name '*test*.py' -path '*/debugger/*' | head -5Repository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Check if there's a way source_file gets populated that we're missing
rg -n 'source_file\s*:' python/mqt/debugger/dap/dap_server.py -B 5 -A 15Repository: munich-quantum-toolkit/debugger
Length of output: 765
🏁 Script executed:
# Let's verify the actual crash scenario - access self.source_file when it's not set
python3 << 'EOF'
# Simulate what happens when accessing an uninitialized attribute
class DAPServer:
host: str
source_file: dict # Type annotation only, no initialization
def __init__(self):
self.host = "127.0.0.1"
# source_file is NOT initialized here
try:
server = DAPServer()
print(f"Trying to access source_file: {server.source_file}")
except AttributeError as e:
print(f"ERROR: {e}")
EOFRepository: munich-quantum-toolkit/debugger
Length of output: 161
Initialize source_file and add exception handling for highlight emission to prevent server crashes.
source_file is declared but never initialized in __init__, so accessing self.source_file at line 258 will raise AttributeError. Even if initialized, HighlightError.__init__ calls _normalize_source() which validates that source has required 'name' and 'path' keys (raises TypeError/ValueError if missing), and validate() requires each highlight to have a non-empty 'message' field. Any of these exceptions will crash the debug session because there is no exception handling around the highlight emission.
Wrap highlight creation and emission in try/except to catch TypeError and ValueError, and use finally to clear pending_highlights even on failure:
Proposed fix
self.columns_start_at_one = True
self.pending_highlights: list[dict[str, Any]] = []
self._prevent_exit = False
+ self.source_file = {"name": "", "path": ""} if self.pending_highlights:
- highlight_event = mqt.debugger.dap.messages.HighlightError(self.pending_highlights, self.source_file)
- send_message(json.dumps(highlight_event.encode()), connection)
- self.pending_highlights = []
- self._prevent_exit = True
+ try:
+ highlight_event = mqt.debugger.dap.messages.HighlightError(self.pending_highlights, self.source_file)
+ send_message(json.dumps(highlight_event.encode()), connection)
+ self._prevent_exit = True
+ except (TypeError, ValueError):
+ pass
+ finally:
+ self.pending_highlights = []Also applies to: 327, 359
| def queue_parse_error(self, error_message: str) -> None: | ||
| """Store highlight data for a parse error to be emitted later.""" | ||
| line, column, detail = self._parse_error_location(error_message) | ||
| entry = self._build_parse_error_highlight(line, column, detail) | ||
| if entry is not None: | ||
| self.pending_highlights = [entry] | ||
|
|
||
| @staticmethod | ||
| def _parse_error_location(error_message: str) -> tuple[int, int, str]: | ||
| """Parse a compiler error string and extract the source location.""" | ||
| match = re.match(r"<input>:(\d+):(\d+):\s*(.*)", error_message.strip()) | ||
| if match: | ||
| line = int(match.group(1)) | ||
| column = int(match.group(2)) | ||
| detail = match.group(3).strip() | ||
| else: | ||
| line = 1 | ||
| column = 1 | ||
| detail = error_message.strip() | ||
| return (line, column, detail) | ||
|
|
||
| def _build_parse_error_highlight(self, line: int, column: int, detail: str) -> dict[str, Any] | None: | ||
| """Create a highlight entry for a parse error.""" | ||
| if not getattr(self, "source_code", ""): | ||
| return None | ||
| lines = self.source_code.split("\n") | ||
| if not lines: | ||
| return None | ||
| line = max(1, min(line, len(lines))) | ||
| column = max(1, column) | ||
| line_index = line - 1 | ||
| line_text = lines[line_index] | ||
|
|
||
| if column <= 1 and line_index > 0 and not line_text.strip(): | ||
| prev_index = line_index - 1 | ||
| while prev_index >= 0 and not lines[prev_index].strip(): | ||
| prev_index -= 1 | ||
| if prev_index >= 0: | ||
| line_index = prev_index | ||
| line = line_index + 1 | ||
| line_text = lines[line_index] | ||
| stripped = line_text.lstrip() | ||
| column = max(1, len(line_text) - len(stripped) + 1) if stripped else 1 | ||
|
|
||
| end_column = max(column, len(line_text) + 1) | ||
| snippet = line_text.strip() or line_text | ||
| return { | ||
| "instruction": -1, | ||
| "range": { | ||
| "start": {"line": line, "column": column}, | ||
| "end": {"line": line, "column": end_column if end_column > 0 else column}, | ||
| }, | ||
| "reason": "parseError", | ||
| "code": snippet, | ||
| "message": detail, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the HighlightError class and its validate method
find . -type f -name "*.py" | xargs grep -l "HighlightError" | head -20Repository: munich-quantum-toolkit/debugger
Length of output: 227
🏁 Script executed:
# Check the current state of the dap_server.py file at the specified lines
fd "dap_server.py" --type fRepository: munich-quantum-toolkit/debugger
Length of output: 143
🏁 Script executed:
cat -n python/mqt/debugger/dap/messages/highlight_error_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 8111
🏁 Script executed:
# Get the exact lines from dap_server.py
sed -n '497,552p' python/mqt/debugger/dap/dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 2381
🏁 Script executed:
# Search for validate method in the HighlightError or related classes
rg "class HighlightError" -A 50 python/mqt/debugger/dap/messages/Repository: munich-quantum-toolkit/debugger
Length of output: 5335
🏁 Script executed:
# Search for where HighlightError.validate() is called
rg "\.validate\(\)" python/mqt/debugger/dap/ -B 3 -A 1Repository: munich-quantum-toolkit/debugger
Length of output: 787
🏁 Script executed:
# Check test file for parse error test cases
sed -n '1,50p' test/python/test_dap_server.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1707
🏁 Script executed:
# Search for queue_parse_error in tests
rg "queue_parse_error\|parse_error" test/python/ -A 3Repository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Search for tests that might exercise parse errors more broadly
rg "parse.*error\|error_message" test/python/ -iRepository: munich-quantum-toolkit/debugger
Length of output: 57
🏁 Script executed:
# Check where HighlightError is instantiated with highlights
rg "HighlightError\(" python/mqt/debugger/dap/ -B 2 -A 5Repository: munich-quantum-toolkit/debugger
Length of output: 2389
🏁 Script executed:
# Verify the initialization chain - DAPEvent.__init__
cat -n python/mqt/debugger/dap/messages/dap_event.pyRepository: munich-quantum-toolkit/debugger
Length of output: 1383
🏁 Script executed:
# Trace how pending_highlights are set and verify the flow
sed -n '1,100p' python/mqt/debugger/dap/dap_server.py | head -60Repository: munich-quantum-toolkit/debugger
Length of output: 1511
🏁 Script executed:
# Confirm how _normalize_highlight processes the message field
sed -n '100,110p' python/mqt/debugger/dap/messages/highlight_error_dap_message.pyRepository: munich-quantum-toolkit/debugger
Length of output: 511
Ensure parse error messages are never empty to prevent validation failures.
The HighlightError.validate() method explicitly rejects empty message fields (line 58-60 of highlight_error_dap_message.py). However, _parse_error_location() can return an empty detail string if the error_message is empty or whitespace-only, which then becomes the message field in the highlight dict. When HighlightError() is instantiated with this dict, validate() is called automatically via DAPEvent.__init__() and raises a ValueError.
Add fallback strings to ensure detail is never empty:
Proposed fix
@@
def queue_parse_error(self, error_message: str) -> None:
@@
line, column, detail = self._parse_error_location(error_message)
+ if not detail:
+ detail = "Parse error."
entry = self._build_parse_error_highlight(line, column, detail)
if entry is not None:
self.pending_highlights = [entry]
@@
def _parse_error_location(error_message: str) -> tuple[int, int, str]:
@@
else:
line = 1
column = 1
- detail = error_message.strip()
+ detail = error_message.strip() or "Parse error."
return (line, column, detail)🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/dap_server.py around lines 497 - 552, The parse
error detail can be empty causing HighlightError.validate() to fail; update
_parse_error_location(error_message) to return a non-empty fallback (e.g.,
"parse error" or "unknown parse error") when detail is empty or only whitespace,
and ensure queue_parse_error/_build_parse_error_highlight will propagate that
non-empty string (alternatively also guard in _build_parse_error_highlight by
replacing an empty detail with the same fallback) so the "message" field in the
highlight dict is never empty.
| @staticmethod | ||
| def _normalize_position(position: Mapping[str, Any] | None) -> dict[str, int]: | ||
| """Normalize a position mapping, ensuring it includes a line and column. | ||
| Args: | ||
| position (Mapping[str, Any] | None): The position mapping to normalize. | ||
| Returns: | ||
| dict[str, int]: A normalized position with integer line and column. | ||
| Raises: | ||
| TypeError: If the provided position is not a mapping. | ||
| ValueError: If required keys are missing. | ||
| """ | ||
| if not isinstance(position, Mapping): | ||
| msg = "Highlight positions must be mappings with 'line' and 'column'." | ||
| raise TypeError(msg) | ||
| try: | ||
| line = int(position["line"]) | ||
| column = int(position["column"]) | ||
| except KeyError as exc: | ||
| msg = "Highlight positions require 'line' and 'column'." | ||
| raise ValueError(msg) from exc | ||
| return { | ||
| "line": line, | ||
| "column": column, | ||
| } |
There was a problem hiding this comment.
Either disallow None in _normalize_position type or handle it explicitly (current signature implies None is acceptable).
Possible adjustment
- def _normalize_position(position: Mapping[str, Any] | None) -> dict[str, int]:
+ def _normalize_position(position: Mapping[str, Any]) -> dict[str, int]:
@@
- if not isinstance(position, Mapping):
+ if not isinstance(position, Mapping):
msg = "Highlight positions must be mappings with 'line' and 'column'."
raise TypeError(msg)🤖 Prompt for AI Agents
In @python/mqt/debugger/dap/messages/highlight_error_dap_message.py around lines
111 - 137, The _normalize_position signature says it accepts Mapping[str, Any] |
None but the implementation doesn't handle None; either remove | None from the
type hint on _normalize_position to disallow None, or explicitly handle None at
the start of the function (e.g., if position is None: raise TypeError("Highlight
positions must be mappings with 'line' and 'column'.") ) so the runtime behavior
matches the signature; update the function annotation or add the explicit None
check inside _normalize_position accordingly.
| } | ||
|
|
||
| Result ddsimRunAll(SimulationState* self, size_t* failedAssertions) { | ||
| auto* ddsim = toDDSimulationState(self); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused local variable in ddsimRunAll.
The ddsim local pointer is obtained but never used in the function body. All operations use the self pointer directly. This appears to be either incomplete refactoring or preparation for future changes.
♻️ Consider removing unused variable
If the local pointer isn't needed for future changes in this function, consider removing it:
Result ddsimRunAll(SimulationState* self, size_t* failedAssertions) {
- auto* ddsim = toDDSimulationState(self);
size_t errorCount = 0;
while (!self->isFinished(self)) {Or if it's intended for future use, add a comment explaining why it's currently unused.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto* ddsim = toDDSimulationState(self); | |
| Result ddsimRunAll(SimulationState* self, size_t* failedAssertions) { | |
| size_t errorCount = 0; | |
| while (!self->isFinished(self)) { |
🤖 Prompt for AI Agents
In @src/backend/dd/DDSimDebug.cpp at line 1153, In ddsimRunAll, the local
pointer 'ddsim' assigned via toDDSimulationState(self) is never used; either
remove the unused declaration entirely or, if you plan to use it later, add a
clear comment above it explaining it's intentionally unused or explicitly mark
it as used (e.g., (void)ddsim) to avoid warnings; update the code around the
'ddsim' declaration accordingly.
| def test_build_highlight_entry_does_not_span_next_instruction() -> None: | ||
| """Ensure highlight ranges stop at the end of the instruction.""" | ||
| server = DAPServer() | ||
| server.source_code = "measure q[0] -> c[0];\nmeasure q[1] -> c[1];\n" | ||
| first_line_end = server.source_code.index("\n") | ||
| fake_diagnostics = SimpleNamespace(potential_error_causes=list) | ||
| fake_state = SimpleNamespace( | ||
| get_instruction_position=lambda _instr: (0, first_line_end), | ||
| get_diagnostics=lambda: fake_diagnostics, | ||
| ) | ||
| server.simulation_state = fake_state # type: ignore[assignment] | ||
|
|
||
| entries = server.collect_highlight_entries(0) | ||
| assert entries | ||
| entry = entries[0] | ||
| assert entry["range"]["start"]["line"] == 1 | ||
| assert entry["range"]["end"]["line"] == 1 |
There was a problem hiding this comment.
Test intent is solid; consider making potential_error_causes explicitly return [] for readability.
Optional tweak
- fake_diagnostics = SimpleNamespace(potential_error_causes=list)
+ fake_diagnostics = SimpleNamespace(potential_error_causes=lambda: [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_build_highlight_entry_does_not_span_next_instruction() -> None: | |
| """Ensure highlight ranges stop at the end of the instruction.""" | |
| server = DAPServer() | |
| server.source_code = "measure q[0] -> c[0];\nmeasure q[1] -> c[1];\n" | |
| first_line_end = server.source_code.index("\n") | |
| fake_diagnostics = SimpleNamespace(potential_error_causes=list) | |
| fake_state = SimpleNamespace( | |
| get_instruction_position=lambda _instr: (0, first_line_end), | |
| get_diagnostics=lambda: fake_diagnostics, | |
| ) | |
| server.simulation_state = fake_state # type: ignore[assignment] | |
| entries = server.collect_highlight_entries(0) | |
| assert entries | |
| entry = entries[0] | |
| assert entry["range"]["start"]["line"] == 1 | |
| assert entry["range"]["end"]["line"] == 1 | |
| def test_build_highlight_entry_does_not_span_next_instruction() -> None: | |
| """Ensure highlight ranges stop at the end of the instruction.""" | |
| server = DAPServer() | |
| server.source_code = "measure q[0] -> c[0];\nmeasure q[1] -> c[1];\n" | |
| first_line_end = server.source_code.index("\n") | |
| fake_diagnostics = SimpleNamespace(potential_error_causes=lambda: []) | |
| fake_state = SimpleNamespace( | |
| get_instruction_position=lambda _instr: (0, first_line_end), | |
| get_diagnostics=lambda: fake_diagnostics, | |
| ) | |
| server.simulation_state = fake_state # type: ignore[assignment] | |
| entries = server.collect_highlight_entries(0) | |
| assert entries | |
| entry = entries[0] | |
| assert entry["range"]["start"]["line"] == 1 | |
| assert entry["range"]["end"]["line"] == 1 |
🤖 Prompt for AI Agents
In @test/python/test_dap_server.py around lines 28 - 44, The fake diagnostics
object uses potential_error_causes=list (the type) which is confusing; change
fake_diagnostics to set potential_error_causes to an actual empty list (e.g.,
potential_error_causes=[]) so the
test_build_highlight_entry_does_not_span_next_instruction more clearly supplies
a diagnostics object with no causes when server.simulation_state.get_diagnostics
is used by DAPServer.collect_highlight_entries.
2 participants
Description
This pull request improves the debugging highlight functionality and fixes an issue where greyed-out highlights were not correctly reset between executions.
When an issue is detected in the code, the extension highlights the affected line in red and displays a descriptive error message explaining the nature of the problem. This visual feedback helps users quickly identify both the location of the error and its underlying cause.
A second part of the implementation concerns the assertion functionality. After an assertion failure, the extension greys out all lines of code that cannot have caused the error. While this behaviour is intended, the greyed-out highlights were previously not cleared when a new execution started. As a result, lines remained greyed out until the editor was closed and reopened.
Overall, the highlighting functionality provides an efficient way to show where an error occurred and what caused it. This change introduces an explicit reset of all greyed-out highlights on launch and restart events, ensuring that each execution starts with a clean editor state.
Fixes #(issue)
Checklist: